Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for FreeBSD #48

Merged
merged 8 commits into from
Jan 15, 2025
Merged

Add support for FreeBSD #48

merged 8 commits into from
Jan 15, 2025

Conversation

smortex
Copy link
Collaborator

@smortex smortex commented Sep 3, 2024

More local patches I have been using on my FreeBSD systems and are worth merging upstream.

@smortex smortex requested a review from a team as a code owner September 3, 2024 18:56
Using Hiera hierarchy, we can provide a default value that makes sense
for the current operating system and still allow the user to provide
their own custom package name if necessary.

Makes the code simpler without changing the logic.
This directory is operating-system dependant.
These directory only make sense on some operating systems and should
already be part of base system or the syslog-ng package.  Do need to
manage these in the patterndb module.
According to hier(7), /var/cache/ is used to store miscellaneous cached
files, precisely what merged patterns not yet installed are.
@smortex
Copy link
Collaborator Author

smortex commented Jan 13, 2025

Looks like CentOS images are gone. Time to remove support for this EOL system. I will open a PR to do so on top of this one.

Copy link
Member

@faxm0dem faxm0dem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @smortex I understand you are improving the semantics of the variable name here. While this is fine by me, it will require a major version bump.

Question: why do you change the smoke tests' base_dir too ? The reason I did this in the first place is so I could run puppet unprivileged for smoke testing.

@smortex
Copy link
Collaborator Author

smortex commented Jan 14, 2025

Thanks @smortex I understand you are improving the semantics of the variable name here. While this is fine by me, it will require a major version bump.

Question: why do you change the smoke tests' base_dir too ? The reason I did this in the first place is so I could run puppet unprivileged for smoke testing.

Ah! I failed to understand this purpose. The different default value and value used in the smoke tests, and the weird value in the test suite just confused me :-)

Now, I see your point, when there was no CI, it was indeed necessary. Now the integration tests make our life easier, so it makes sense to remove it completely.

While this is fine by me, it will require a major version bump.

Correct, I removed a parameter, so will change the PR accordingly. I just spot that I forgot to remove the default value from Hiera so will add a commit to finish this properly.

The variable was removed, so no need for a default value and we should
not see references to it in the README.
@smortex
Copy link
Collaborator Author

smortex commented Jan 14, 2025

The README is still not exactly up-to-date. puppet-string will help to keep things in sync, but it is a big task and this PR is already big, so I suggest to switching to it when this PR is merged.

@smortex smortex requested a review from faxm0dem January 14, 2025 18:33
Copy link
Member

@faxm0dem faxm0dem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I agree the base_dir isn't needed for CI, there are people still left in this world (guess who) who like to use actual bare metal containerless machines to actually run some software.
Why not name it _base_dir to stress it's undocumented/unsupported but still leave it around for those rare nerds who like to do it "the old way" ? 👼

```

This *File* is generated by merging all defined *ruleset resources*, which come in two forms: [raw](#defined-type-patterndbrawruleset) and [simple](#defined-type-patterndbsimpleruleset).
Merging is handled under the hood by using `pdbtool merge` which creates a new *patterndb parser* in the `${temp_dir}` directory. Testing of the merged *parser* is optionally handled using `pdbtool test`. If this is a success, the merged file is then being deployed into the definitive destination at `${base_dir}/var/lib/syslog-ng/patterndb/${name}.xml`.
Merging is handled under the hood by using `pdbtool merge` which creates a new *patterndb parser* in the `${temp_dir}` directory. Testing of the merged *parser* is optionally handled using `pdbtool test`. If this is a success, the merged file is then being deployed into the definitive destination at `/var/lib/syslog-ng/patterndb/${name}.xml`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed one here

@smortex smortex merged commit 8c8b715 into master Jan 15, 2025
16 of 20 checks passed
@smortex
Copy link
Collaborator Author

smortex commented Jan 15, 2025

while I agree the base_dir isn't needed for CI, there are people still left in this world (guess who) who like to use actual bare metal containerless machines to actually run some software. Why not name it _base_dir to stress it's undocumented/unsupported but still leave it around for those rare nerds who like to do it "the old way" ? 👼

Of course I see this message only after merging the approved PR that include it 😑

I'll open a PR to revert it.

@smortex
Copy link
Collaborator Author

smortex commented Jan 15, 2025

I'll open a PR to revert it.

Question: rather than prepending ${base_dir} to all paths, would it not make sense to rather prepend that value to the actual 3 parameter that use it:

$base_dir = '/tmp/x'

class { 'patterndb':
  cache_dir  => "${base_dir}/var/cache/syslog-ng",
  var_dir    => "${base_dir}/var",
  config_dir => "${base_dir}/etc/patterndb.d",
}

That still* does not work out-of-the-box because /tmp/xx needs a bunch of sub-directories that are not managed by the module, but is it something that would help you? If not, can you describe your use-case?

@faxm0dem
Copy link
Member

yeah that would be all right for my use-case.
As for managing the subdirs... I think I can live with that - creating them manually would fit my use-case too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants